-
Notifications
You must be signed in to change notification settings - Fork 5.3k
Fix integer overflow in ArrayList.IListWrapper.BinarySearch and improve test coverage #123806
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
|
Tagging subscribers to this area: @dotnet/area-system-collections |
Co-authored-by: vcsjones <361677+vcsjones@users.noreply.github.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
This PR fixes an integer overflow vulnerability in ArrayList.IListWrapper.BinarySearch by replacing the overflow-prone midpoint calculation with the overflow-safe pattern already used throughout the .NET runtime.
Changes:
- Replace
mid = (lo + hi) / 2withmid = lo + ((hi - lo) >> 1)to prevent integer overflow when searching large arrays - Move the
midvariable declaration inside the while loop to limit its scope
src/libraries/System.Private.CoreLib/src/System/Collections/ArrayList.cs
Show resolved
Hide resolved
Add BinarySearch_LargeList_NoIntegerOverflow test that verifies the fix handles large list indices correctly without integer overflow. The test uses a simulated large list to avoid memory allocation issues in CI. Co-authored-by: stephentoub <2642209+stephentoub@users.noreply.github.com>
src/libraries/System.Private.CoreLib/src/System/Collections/ArrayList.cs
Show resolved
Hide resolved
src/libraries/System.Collections.NonGeneric/tests/ArrayListTests.cs
Outdated
Show resolved
Hide resolved
Add 7 additional tests to improve coverage: - BinarySearch_EmptyList: Test searching in empty list - BinarySearch_SingleElement: Test single element edge case - BinarySearch_BoundaryConditions: Test search at start/end of ranges - BinarySearch_PartialRangeSearch: Test searching within subranges - BinarySearch_ComparerThrowsException: Test exception propagation from comparer - BinarySearch_UnsortedList: Test behavior on unsorted list (should not crash) - BinarySearch_TwoElementList: Test two-element edge case All 20 BinarySearch tests now pass. Co-authored-by: stephentoub <2642209+stephentoub@users.noreply.github.com>
Co-authored-by: stephentoub <2642209+stephentoub@users.noreply.github.com>
Co-authored-by: stephentoub <2642209+stephentoub@users.noreply.github.com>
Description
The
IListWrapper.BinarySearchimplementation computed the midpoint asmid = (lo + hi) / 2, which overflows when searching large arrays (e.g.,byte[int.MaxValue / 2 + 2]). This PR fixes the overflow issue and significantly improves test coverage for the BinarySearch method.Fixes #123804
Changes
Array.GetMedian:mid = lo + ((hi - lo) >> 1)middeclaration inside the loopBinarySearch_LargeList_NoIntegerOverflowthat validates the fix using a simulated large IList to avoid memory allocation issues in CIBinarySearch_EmptyList: Test searching in empty listBinarySearch_SingleElement: Test single element edge caseBinarySearch_BoundaryConditions: Test search at start/end of rangesBinarySearch_PartialRangeSearch: Test searching within subrangesBinarySearch_ComparerThrowsException: Test exception propagation from comparerBinarySearch_UnsortedList: Test behavior on unsorted list (should not crash)BinarySearch_TwoElementList: Test two-element edge caseThrowingComparerhelper class for exception testingFakeLargeIListhelper class that simulates an array ofint.MaxValue / 2 + 2elements without actually allocating memory, making it suitable for CI environmentsThis matches the pattern already used in
Array.BinarySearchand prevents overflow by ensuring(hi - lo)remains in bounds before the division.Additional Investigation
Investigated other types in the repository for similar overflow risks. Found that:
Array.csandArraySortHelper.csalready use the safe patternExpressionParser.cs(System.Data.Common) has the unsafe pattern but very low risk (small fixed array)ConcurrentSet.cs(TraceLogging) has the unsafe pattern with medium theoretical riskTesting
💡 You can make Copilot smarter by setting up custom instructions, customizing its development environment and configuring Model Context Protocol (MCP) servers. Learn more Copilot coding agent tips in the docs.